Add deny assignment management by deads2k · Pull Request #6680 · Azure/ARO-HCP

David Eads (deads2k) · 2026-08-25T15:17:25Z

/assign Manyanda Chitimbo (@machi1990)

Copilot

Pull request overview

This PR introduces backend-side management of Azure deny assignments for HCP clusters, including tracking their lifecycle in ServiceProviderCluster and gating Cluster Service creation until deny assignments are in place (when running with a real First Party Application).

Changes:

Add a new ClusterDenyAssignment controller to create/update/delete deny assignments in the cluster’s managed resource group and persist state into Cosmos (ServiceProviderCluster.Status.AzureResources.DenyAssignments).
Extend the ServiceProviderCluster API model to track deny assignment references (pending vs confirmed) and add helper functions for deterministic deny assignment resource IDs.
Gate cluster creation dispatch on deny assignment readiness when the environment supports a real FPA (and explicitly skip that gate when it does not).

Reviewed changes

Copilot reviewed 16 out of 18 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
internal/api/coreapi/zz_generated.deepcopy.go	Adds deepcopy support for new deny assignment reference types.
internal/api/coreapi/types_serviceprovider_cluster.go	Replaces `denyAssignments` from `AzureMultiReference` to typed deny assignment reference tracking.
internal/api/coreapi/types_cosmosdata.go	Adds helpers to build/parse deny assignment resource IDs.
backend/pkg/utils/controllerutils/util.go	Adds helper to consistently extract Cluster Service ID (pending vs assigned).
backend/pkg/controllers/cluster/denyassignments/deny_assignment_permissions.go	Defines deny-assignment action/notAction sets per RP area.
backend/pkg/controllers/cluster/denyassignments/deny_assignment_definitions.go	Defines deny assignment types and computes required references per cluster.
backend/pkg/controllers/cluster/denyassignments/deny_assignment_controller.go	Implements ClusterDenyAssignment controller reconciliation and Azure upsert/delete logic.
backend/pkg/controllers/cluster/denyassignments/deny_assignment_controller_test.go	Adds unit tests for deny assignment controller logic and UUID derivation.
backend/pkg/controllers/cluster/deletion/cluster_child_resources_cleanup_controller.go	Avoids deletion gating on deny assignment tracking (cascade deletion via MRG).
backend/pkg/controllers/cluster/creation/cluster_cluster_service_create_controller.go	Adds precondition: deny assignments created (only when enabled).
backend/pkg/controllers/cluster/creation/cluster_cluster_service_create_controller_test.go	Updates/extends creation tests to include deny assignment precondition behavior.
backend/pkg/azure/client/mock_fpa_client_builder.go	Extends gomock builder to support DenyAssignments/GenericResources clients.
backend/pkg/azure/client/generic_resources_client.go	Introduces `GenericResourcesClient` interface abstraction for ARM generic resources.
backend/pkg/azure/client/fpa_client_builder.go	Adds FPA client construction for DenyAssignments and GenericResources.
backend/pkg/azure/client/deny_assignments_client.go	Introduces `DenyAssignmentsClient` interface abstraction.
backend/pkg/azure/azuremockclient/mock_clients.go	Adds lightweight mock Azure clients for deny assignments + generic resources.
backend/pkg/app/backend.go	Wires the new deny assignment controller conditionally (real FPA only).
backend/cmd/root.go	Plumbs `HasRealFPA` into backend options to enable/disable deny assignments behavior.

Files not reviewed (2)

backend/pkg/azure/client/mock_fpa_client_builder.go: Generated file
internal/api/coreapi/zz_generated.deepcopy.go: Generated file

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

 // AzureResources groups the Azure resource references associated with a cluster.
 type AzureResources struct {
 	// DenyAssignments tracks the deny assignments applied to the cluster's resources.
-	DenyAssignments AzureMultiReference `json:"denyAssignments,omitempty"`
+	DenyAssignments DenyAssignmentReferences `json:"denyAssignments,omitempty"`
 	// ManagedResourceGroup tracks the managed resource group for the cluster.
 	ManagedResourceGroup AzureReference `json:"managedResourceGroup,omitempty"`


+// GenericResourcesClientFunc adapts functions to the GenericResourcesClient interface.
+// Tests set the function fields to control the response.
+// BeginCreateOrUpdateByID and BeginDeleteByID return a nil *Poller and an error — to simulate
+// success, return (nil, nil) and the calling code will call PollUntilDone on nil.
+// To avoid that, the tests should exercise paths that don't reach PollUntilDone (e.g. error paths)
+// or the mock should capture the call without returning a real poller.
+//
+// For paths that call PollUntilDone, set CreateErr/DeleteErr to non-nil to prevent the nil-pointer dereference.
+type GenericResourcesClientFunc struct {


Manyanda Chitimbo (machi1990) · 2026-08-25T16:00:00Z

+	var clusterDenyAssignmentController controllerutils.Controller
+	if b.options.HasRealFPA {
+		clusterDenyAssignmentController = denyassignments.NewClusterDenyAssignmentController(
+			utilsclock.RealClock{},
+			b.options.ResourcesDBClient,
+			b.options.FPAClientBuilder,
+			backendInformers,
+		)
+	}


Nitpick and it can be a followup;

Always running this controller even in environments where we don't have a real FPA and in those envs, we NOOP the calls but tracks the azure resources ids etc so that we can visualise verify the controller running correctly

Manyanda Chitimbo (machi1990) · 2026-08-25T16:03:12Z

+	// Deny assignments are scoped to the managed resource group, so Azure deletes them in cascade
+	// when that resource group is removed during cluster teardown; the ClusterDenyAssignment
+	// controller therefore does nothing on delete and never clears these references. Gating here
+	// would block cleanup forever. (Per Manyanda Karombi's note on


Suggested change

// would block cleanup forever. (Per Manyanda Karombi's note on

// would block cleanup forever. (Per Manyanda Chitimbo's note on

:-)

Manyanda Chitimbo (machi1990) · 2026-08-25T16:08:19Z

+	// Nothing to do while the cluster is being deleted. The deny assignments are scoped to the
+	// managed resource group, so Azure deletes them in cascade when that resource group is removed
+	// during cluster teardown; there is no need to issue ARM deletions or otherwise reconcile them
+	// here. (Per Manyanda Karombi's note on https://github.com/Azure/ARO-HCP/pull/6269#discussion_r3656341978.)


Suggested change

// here. (Per Manyanda Karombi's note on https://github.com/Azure/ARO-HCP/pull/6269#discussion_r3656341978.)

// here. (Per Manyanda Chitimbo's note on https://github.com/Azure/ARO-HCP/pull/6269#discussion_r3656341978.)

Manyanda Chitimbo (machi1990) · 2026-08-25T16:20:38Z

+	lookup := make(map[string]string, len(cluster.Identity.UserAssignedIdentities))
+	for resourceID, identity := range cluster.Identity.UserAssignedIdentities {
+		if identity != nil && identity.PrincipalID != nil {
+			lookup[strings.ToLower(resourceID)] = *identity.PrincipalID
+		}
+	}


The principalIDs of the data plane identities won't be in this list; this list only contains the identities that are resolvable via MSI RP i.e the control plane identities + service managed identity.

For data plane identies, we need to resolve the principal ID from https://github.com/deads2k/ARO-HCP/blob/46e74dc6c8cdc8f918e94bb0921d63ff767ffdcf/internal/api/coreapi/types_serviceprovider_cluster.go#L270

While addressing this, you could consider also reading the CP + SMI ones from https://github.com/deads2k/ARO-HCP/blob/46e74dc6c8cdc8f918e94bb0921d63ff767ffdcf/internal/api/coreapi/types_serviceprovider_cluster.go#L260

Manyanda Chitimbo (machi1990) · 2026-08-25T16:24:37Z

+	return c.syncDenyAssignmentUpsert(ctx, key, cluster)
+}
+
+func (c *clusterDenyAssignmentSyncer) syncDenyAssignmentNeedsWork(cluster *coreapi.HCPOpenShiftCluster, serviceProviderCluster *coreapi.ServiceProviderCluster) bool {


Also check for

https://github.com/deads2k/ARO-HCP/blob/46e74dc6c8cdc8f918e94bb0921d63ff767ffdcf/internal/api/coreapi/types_serviceprovider_cluster.go#L260 and https://github.com/deads2k/ARO-HCP/blob/46e74dc6c8cdc8f918e94bb0921d63ff767ffdcf/internal/api/coreapi/types_serviceprovider_cluster.go#L270 contain non empty identities

i.e len(.DataPlaneOperatorsManagedIdentities.Identies) > 0 , similarly for CP + SMI so that this controller only runs when the principalIDs of those operators have been resolved to avoid this piece https://github.com/deads2k/ARO-HCP/blob/46e74dc6c8cdc8f918e94bb0921d63ff767ffdcf/backend/pkg/controllers/cluster/denyassignments/deny_assignment_controller.go#L360 returning an error for an expected situation during cluster creation becaus the identities are eventually synced

Adds a controller that manages Azure deny assignments on the managed resource group for each HCP cluster. The controller ensures all required deny assignments exist with correct content, deletes stale ones, and periodically rechecks consistency with jittered recheck intervals. Deny assignments are scoped to the managed resource group and are cleaned up automatically when the resource group is deleted during cluster teardown. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…ProviderCluster The data-plane operator principal IDs are not present on cluster.Identity, so resolving excluded principals from cluster.Identity.UserAssignedIdentities could never find them. The RP now mirrors both the MSI and data-plane operator identities (with resolved principal IDs) onto the ServiceProviderCluster status. resolvePrincipalIDs now takes the ServiceProviderCluster and matches each excluded identity against Status.MSIManagedIdentities (control plane operators + service managed identity) and Status.DataPlaneOperatorsManagedIdentities, returning an error when no resolved principal ID is found so the sync retries rather than writing a deny assignment with a missing exclusion. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…signments Excluded principal IDs are resolved from the identities the MSI and data-plane identity controllers mirror onto the ServiceProviderCluster. When those maps are empty the resolution can only fail, so syncDenyAssignmentNeedsWork now returns false until both ControlPlaneOperatorsIdentities and the data-plane Identities maps are populated, letting the resolution controllers run first instead of churning on unresolvable principals. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 16 out of 18 changed files in this pull request and generated 5 comments.

Files not reviewed (2)

backend/pkg/azure/client/mock_fpa_client_builder.go: Generated file
internal/api/coreapi/zz_generated.deepcopy.go: Generated file

Suppressed comments (3)

backend/pkg/controllers/cluster/denyassignments/deny_assignment_controller.go:574

Use the new de-duplicating helper when adding identities to the excluded list.

		identityResourceIDs = append(identityResourceIDs, resourceID)

backend/pkg/controllers/cluster/denyassignments/deny_assignment_controller.go:581

Use the new de-duplicating helper when adding identities to the excluded list.

		identityResourceIDs = append(identityResourceIDs, identities.ServiceManagedIdentity)

backend/pkg/azure/azuremockclient/mock_clients.go:80

This mock currently forces an error any time CreateErr/DeleteErr is nil, which prevents unit tests from exercising the controller’s successful PollUntilDone paths (and makes it easy to accidentally couple tests to error-only behavior). Consider refactoring the client interfaces to return a small poller interface (or otherwise injecting a fake poller) so tests can cover the success cases too.

	if m.CreateErr != nil {
		return nil, m.CreateErr
	}
	return nil, fmt.Errorf("GenericResourcesClientFunc: set CreateErr to control this path; PollUntilDone cannot be called on a nil poller")
}

+	PendingAzureResources []DenyAssignmentReference `json:"pendingDenyAssignments,omitempty"`
+	// AzureResources contains resource IDs that have been confirmed to exist in Azure.
+	// Written by: ClusterDenyAssignment
+	AzureResources []DenyAssignmentReference `json:"denyAssignments,omitempty"`


+	// HasRealFPA indicates the backend runs against a real First Party Application rather than the
+	// insecure MI mock. Controllers that create Azure resources only a real FPA can create (e.g.
+	// deny assignments) are disabled when this is false (dev/int environments).
+	HasRealFPA                                          bool
 	BackendIdentityAzureClients                         *azureclient.BackendIdentityAzureClients


+func (c *clusterDenyAssignmentSyncer) syncDenyAssignmentUpsert(ctx context.Context, key controllerutils.HCPClusterKey, cluster *coreapi.HCPOpenShiftCluster) error {
+	logger := utils.LoggerFromContext(ctx)
+
+	serviceProviderCluster, err := corecosmosstorage.GetOrCreateServiceProviderCluster(ctx, c.resourcesDBClient, cluster.ID)
+	if err != nil {
+		return utils.TrackError(fmt.Errorf("failed to get or create ServiceProviderCluster: %w", err))


+	var identityResourceIDs []*azcorearm.ResourceID
+
+	identities := cluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities
+


+		if !ok || resourceID == nil {
+			return nil, fmt.Errorf("control plane operator %q not found in cluster identity configuration", operatorName)
+		}
+		identityResourceIDs = append(identityResourceIDs, resourceID)


Manyanda Chitimbo (machi1990)

/lgtm

openshift-ci · 2026-08-26T15:34:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, machi1990

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~backend/OWNERS~~ [deads2k]
~~internal/OWNERS~~ [deads2k]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-merge-bot · 2026-08-26T16:14:30Z

/retest-required

Remaining retests: 0 against base HEAD 9cc2e1d and 2 for PR HEAD 0fd4d7b in total

openshift-merge-bot · 2026-08-26T22:07:32Z

/retest-required

Remaining retests: 0 against base HEAD 2d7a67b and 1 for PR HEAD 0fd4d7b in total

openshift-merge-bot · 2026-08-27T06:14:13Z

/retest-required

Remaining retests: 0 against base HEAD 2ed5b65 and 0 for PR HEAD 0fd4d7b in total

openshift-ci · 2026-08-27T07:33:57Z

David Eads (@deads2k): The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-parallel	`0fd4d7b`	link	true	`/test e2e-parallel`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

openshift-merge-bot · 2026-08-27T08:12:39Z

/hold

Revision 0fd4d7b was retested 3 times: holding

Manyanda Chitimbo (machi1990) · 2026-08-27T08:24:52Z

/hold cancel
/retest

Copilot AI lite review requested due to automatic review settings August 25, 2026 15:17

openshift-ci Bot assigned Manyanda Chitimbo (machi1990) Aug 25, 2026

openshift-ci Bot requested review from Jan-Hendrik Boll (janboll) and Miguel Soriano (miguelsorianod) August 25, 2026 15:17

openshift-ci Bot added the approved label Aug 25, 2026

David Eads (deads2k) mentioned this pull request Aug 25, 2026

feat: add ClusterDenyAssignment controller for Azure deny assignments #6269

Closed

Copilot started reviewing on behalf of David Eads (deads2k) August 25, 2026 15:17 View session

Copilot AI reviewed Aug 25, 2026

View reviewed changes

Manyanda Chitimbo (machi1990) reviewed Aug 25, 2026

View reviewed changes

David Eads (deads2k) and others added 4 commits August 25, 2026 17:45

force diff

3779baf

Copilot AI review requested due to automatic review settings August 26, 2026 14:47

David Eads (deads2k) force-pushed the cs-214-deny-2 branch from 46e74dc to 0fd4d7b Compare August 26, 2026 14:47

Copilot started reviewing on behalf of David Eads (deads2k) August 26, 2026 14:48 View session

Copilot AI reviewed Aug 26, 2026

View reviewed changes

Manyanda Chitimbo (machi1990) approved these changes Aug 26, 2026

View reviewed changes

openshift-ci Bot added the lgtm label Aug 26, 2026

openshift-ci Bot added the do-not-merge/hold label Aug 27, 2026

openshift-ci Bot removed the do-not-merge/hold label Aug 27, 2026

	// would block cleanup forever. (Per Manyanda Karombi's note on
	// would block cleanup forever. (Per Manyanda Chitimbo's note on

	// here. (Per Manyanda Karombi's note on https://github.com/Azure/ARO-HCP/pull/6269#discussion_r3656341978.)
	// here. (Per Manyanda Chitimbo's note on https://github.com/Azure/ARO-HCP/pull/6269#discussion_r3656341978.)

		var identityResourceIDs []*azcorearm.ResourceID

		identities := cluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities

Uh oh!

Conversation

David Eads (deads2k) commented Aug 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Manyanda Chitimbo (machi1990) Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

Manyanda Chitimbo (machi1990) Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

Manyanda Chitimbo (machi1990) Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

Manyanda Chitimbo (machi1990) Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

Manyanda Chitimbo (machi1990) Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Manyanda Chitimbo (machi1990) left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented Aug 26, 2026

Uh oh!

openshift-merge-bot Bot commented Aug 26, 2026

Uh oh!

openshift-merge-bot Bot commented Aug 26, 2026

Uh oh!

openshift-merge-bot Bot commented Aug 27, 2026

Uh oh!

openshift-ci Bot commented Aug 27, 2026

Uh oh!

openshift-merge-bot Bot commented Aug 27, 2026

Uh oh!

Manyanda Chitimbo (machi1990) commented Aug 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Manyanda Chitimbo (machi1990) Aug 25, 2026 •

edited

Loading